Selectively allow more leading zeros when outputting feature values #1359
Conversation
…leading zeros which are not handled by 0.6f, instead selectively use more digits in the output, but only if required to maintain compatibility
|
Does the dynamic range of the feature span more than 6 orders of magnitude? If so, then I guess this is the way to go, cc @kylophone for another pair of eyes. In any case, could this be updated to use |
libvmaf/src/output.h
Outdated
| @@ -19,6 +19,8 @@ | |||
| #ifndef __VMAF_OUTPUT_H__ | |||
| #define __VMAF_OUTPUT_H__ | |||
|
|
|||
| int countLeadingZeroesFloat(float x); | |||
There was a problem hiding this comment.
I think this function could be static and then the header doesn't need to declare it
libvmaf/src/output.c
Outdated
| @@ -44,6 +44,26 @@ static const char *pool_method_name[] = { | |||
| [VMAF_POOL_METHOD_HARMONIC_MEAN] = "harmonic_mean", | |||
| }; | |||
|
|
|||
| static int count_leading_zeros_flat(double x) | |||
There was a problem hiding this comment.
nit: I think this used to be float and got typoed to flat? in either case, it is a bit weird to call it float and have it operate on doubles. There are some uses of the "_s for single, _d for double" convention in this repo, such as here - maybe count_leading_zeros_d?
There was a problem hiding this comment.
welp, that was definitely a typo. Renamed to count_leading_zeros_d as suggested.
libvmaf/src/output.c
Outdated
| if(x < 0) | ||
| x = fabs(x); | ||
|
|
||
| int intPart = (int)x; |
There was a problem hiding this comment.
nit: a couple more snake_case candidates
There was a problem hiding this comment.
LGTM, let's give it a few days in case @kylophone has any comments and merge
During the implementation of FUNQUE+ feature extractors we ran into problems with ST-RRED due to the amount of leading zeros required. Even after applying some normalization factors it would still routinely exceed the 0.6f formatting in VMAF. However changing the default feature format to allow more zeros seemed invasive and broke a bunch of tests.
As a result we selectively enabled printing more digits only when required by the actual feature value, while maintaining compatibility with existing feature extractors which don't need this.
This can wait until the FUNQUE+ feature extractor that requires it.
Co-authored-by: Priyanka-885 [email protected]